Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

fix: netlify-edge-functions-test for PRs and jest tests #3229

Merged
merged 5 commits into from
Sep 17, 2024
Merged

fix: netlify-edge-functions-test for PRs and jest tests #3229

merged 5 commits into from
Sep 17, 2024

Conversation

anshgoyalevil
Copy link
Member

@anshgoyalevil anshgoyalevil commented Sep 13, 2024 •
edited
Loading

Description

  • This PR basically fixes some things left in PR test: add tests for netlify functions #2493
  • This PR adds the target where the workflow should be triggered.
  • Also, a minor fix in the test statement is introduced to exclude jest tests from being executed by deno.

@netlify Netlify
Copy link

netlify bot commented Sep 13, 2024 •
edited
Loading

✅ Deploy Preview for asyncapi-website ready!

Built without sensitive environment variables

Name Link
🔨 Latest commit ea2f7de
🔍 Latest deploy log https://app.netlify.com/sites/asyncapi-website/deploys/66e853dbf5f0ff000859cf44
😎 Deploy Preview https://deploy-preview-3229--asyncapi-website.netlify.app
📱 Preview on mobile
Toggle QR Code...

QR Code

Use your smartphone camera to open QR code link.

To edit notification comments on pull requests, go to your Netlify site configuration.

@asyncapi-bot
Copy link
Contributor

asyncapi-bot commented Sep 13, 2024 •
edited
Loading

⚡️ Lighthouse report for the changes in this PR:

Category Score
🔴 Performance 43
🟢 Accessibility 98
🟢 Best practices 92
🟢 SEO 100
🔴 PWA 33

Lighthouse ran on https://deploy-preview-3229--asyncapi-website.netlify.app/

akshatnema
akshatnema reviewed Sep 14, 2024
View reviewed changes
.github/workflows/netlify-edge-functions-test.yml
@@ -1,20 +1,21 @@
name: Run tests for netlify edge-functions

on:
workflow_dispatch
pull_request:
types: [opened, reopened, synchronize, ready_for_review]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is it necessary to test netlify function this many times on each PR?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are we calling in the actual netlify function while testing? Not sure, maybe @Shurtu-gal might help here.
Is that's not the case, then this is okay, since we do that number of times for jest tests also.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The actual netlify function is not called at all. Instead the logic is tested through mocking.

website/netlify/edge-functions/tests/serve-definitions.test.ts

Lines 54 to 76 in 7b23c07

mf.install();
mf.mock("*", (req) => {
console.log(req.url);
if (req.url === metricURL) {
metricCalls++;
}
const body = {
url: req.url,
method: req.method,
headers: req.headers,
};
return new Response(JSON.stringify(body), {
status: 200,
headers: {
"Content-Type": "application/json",
},
});
});
}

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for clarification @Shurtu-gal
@akshatnema I think we can then go for running netlify function tests for every PR this number of times. Afterall, they are taking around 11 seconds

@akshatnema
Copy link
Member

/rtm

@asyncapi-bot asyncapi-bot merged commit 029a88b into asyncapi:master Sep 17, 2024
16 checks passed
@anshgoyalevil anshgoyalevil deleted the netlify-wf-update branch September 17, 2024 17:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Shurtu-gal Shurtu-gal Shurtu-gal left review comments

@akshatnema akshatnema akshatnema approved these changes

@derberg derberg Awaiting requested review from derberg derberg is a code owner

@magicmatatjahu magicmatatjahu Awaiting requested review from magicmatatjahu magicmatatjahu is a code owner

@Mayaleeeee Mayaleeeee Awaiting requested review from Mayaleeeee Mayaleeeee is a code owner

@asyncapi-bot-eve asyncapi-bot-eve Awaiting requested review from asyncapi-bot-eve asyncapi-bot-eve is a code owner

@sambhavgupta0705 sambhavgupta0705 Awaiting requested review from sambhavgupta0705

Assignees
No one assigned
Labels
ready-to-merge
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@anshgoyalevil @asyncapi-bot @akshatnema @Shurtu-gal